Add BlobContainer.writeBlobAtomic()#30902
Conversation
tlrx
added
review
:Distributed/Snapshot/Restore
|
Pinging @elastic/es-distributed |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
I would prefer if you also replace the last occurrence of move, and then remove the move method. In terms of testing, I wonder if we need to inject more failures now so that FSBlobContainer call randomly fail between the write and atomic move.
There was a problem hiding this comment.
There is a relation between the name here and between ChecksumBlobStoreFormat.isTempBlobName, which is now easy to break. I think we should make this more explicit.
I removed the last occurance (but added one in test). I'll remove the method in a follow up pull request if that's fine for you (I expect to remove many things in repository plugins related to this).
I agree. I updated the code, can you have another look please? I also move two tests classes were they should be and fix the checkstyle violations on the way. |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
I've left two more comments.
There was a problem hiding this comment.
s/then/the/
Also it's rather the opposite. If the implementation support atomic writes by default, it then delegates to writeBlob...?
There was a problem hiding this comment.
"When the BlobContainer implementation does not provide a specific implementation for wireBlobAtomic, then the usual writeBlob method is used." Would that be better?
There was a problem hiding this comment.
Should we really ignore all exceptions here? This means we can up with lingering files without any warnings.
I preferred the previous approach of:
} catch (IOException ex) {
// temporary blob creation or move failed - try cleaning up
try {
snapshotsBlobContainer.deleteBlobIgnoringIfNotExists(tempBlobName);
} catch (IOException e) {
ex.addSuppressed(e);
}
throw ex;
}
|
Thanks @ywelsch - I applied your feedback. Can you have another look please? |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
I've left one last comment, o.w. LGTM. Thanks for this.
There was a problem hiding this comment.
this line should be removed now.
There was a problem hiding this comment.
raaah thanks
I should pay a 🍺 every time I forgot something like that
|
@elasticmachine test this please |
This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations. When the repository does not support atomic writes, this method just delegate the write operation to the usual writeBlob() method. Related to elastic#30680
|
Thanks @ywelsch |
This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations. When the BlobContainer implementation does not provide a specific implementation of writeBlobAtomic(), then the writeBlob() method is used. Related to #30680
* elastic/master: [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860) Take into account the return value of TcpTransport.readMessageLength(...) in Netty4SizeHeaderFrameDecoder Move caching of the size of a directory to `StoreDirectory`. (elastic#30581) Clarify docs about boolean operator precedence. (elastic#30808) Docs: remove notes on sparsity. (elastic#30905) Fix MatchPhrasePrefixQueryBuilderTests#testPhraseOnFieldWithNoTerms run overflow forecast a 2nd time as regression test for elastic/ml-cpp#110 (elastic#30969) Improve documentation of dynamic mappings. (elastic#30952) Decouple MultiValueMode. (elastic#31075) Docs: Clarify constraints on scripted similarities. (elastic#31076)
* 6.x: Share common readFrom/writeTo code in AcknowledgeResponse (#30983) [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient Fix rest test skip version Fix docs build. Add a doc value format to binary fields. (#30860) Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Move caching of the size of a directory to `StoreDirectory`. (#30581) Clarify docs about boolean operator precedence. (#30808) Docs: remove notes on sparsity. (#30905) Improve documentation of dynamic mappings. (#30952) Decouple MultiValueMode. (#31075) Docs: Clarify constraints on scripted similarities. (#31076)
* master: Removing erroneous repeat Adapt bwc versions after backporting #30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (#30978) Fix docs build. Only auto-update license signature if all nodes ready (#30859) Add BlobContainer.writeBlobAtomic() (#30902) Add a doc value format to binary fields. (#30860)
* ccr: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (elastic#30491) Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073) Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860)
* ccr: [DOCS] Creates rest-api folder in docs [Rollup] Disallow index patterns that match the rollup index (elastic#30491) Replace exact numDocs by soft-del count in SegmentInfo (elastic#31086) Upgrade to Lucene-7.4.0-snapshot-0a7c3f462f (elastic#31073) Add cors support to NioHttpServerTransport (elastic#30827) [DOCS] Fixes security example (elastic#31082) Allow terms query in _rollup_search (elastic#30973) Removing erroneous repeat Adapt bwc versions after backporting elastic#30983 to 6.4 [Tests] Muting RatedRequestsTests#testXContentParsingIsNotLenient TEST: Retry synced-flush if ongoing ops on primary (elastic#30978) Fix docs build. Only auto-update license signature if all nodes ready (elastic#30859) Add BlobContainer.writeBlobAtomic() (elastic#30902) Add a doc value format to binary fields. (elastic#30860)
4 participants
This commit adds a new writeBlobAtomic() method to the BlobContainer interface that can be implemented by repository implementations which support atomic writes operations.
When the repository does not support atomic writes, this method just delegate the write operation to the usual writeBlob() method.
As a follow up method, I think we could just remove the
BlobContainer.move()method now it's just used during repository verification.Related to #30680